Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Validate AsOfJoin tolerance and attempt interval unit conversion #1952

Merged
merged 6 commits into from
Sep 13, 2019

Conversation

emilyreff7
Copy link
Contributor

@emilyreff7 emilyreff7 commented Sep 6, 2019

Unlike other operations, not all of the arguments of AsOfJoin are automatically passed through the validation chain that occurs in superclass Annotable, since they are not passed into super(). The result is that whereas other operations with offset-like arguments (e.g. Lead/Lag) have their offsets' dtypes automatically inferred, the same does not occur for 'tolerance' in AsOfJoin. We can simply wire this validation in ourselves.

Additionally, the default unit for Interval is 's' regardless of the unit of the value of origin. We can make a better attempt at inferring unit before immediately defaulting to seconds.

ibis/expr/tests/test_table.py Show resolved Hide resolved
ibis/expr/datatypes.py Outdated Show resolved Hide resolved
@toryhaavik
Copy link
Contributor

hopefully #1963 can get in ahead of this, you can rebase on top of it, and then only your enhancement will be there, not the miscellaneous formatting changes too

@codecov
Copy link

codecov bot commented Sep 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ae71b3a). Click here to learn what that means.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1952   +/-   ##
=========================================
  Coverage          ?   86.12%           
=========================================
  Files             ?       93           
  Lines             ?    17116           
  Branches          ?     2164           
=========================================
  Hits              ?    14741           
  Misses            ?     1968           
  Partials          ?      407
Impacted Files Coverage Δ
ibis/expr/operations.py 95.88% <100%> (ø)
ibis/expr/datatypes.py 94.55% <91.3%> (ø)

@toryhaavik
Copy link
Contributor

@emilyreff7 please rebase on top of master and then i'll approve!

@emilyreff7
Copy link
Contributor Author

@emilyreff7 please rebase on top of master and then i'll approve!

Just rebased, looks like only my changes remain :)

@toryhaavik
Copy link
Contributor

these seem like spurious failures, maybe try amending your commit and force pushing to kick off CI again

@emilyreff7
Copy link
Contributor Author

these seem like spurious failures, maybe try amending your commit and force pushing to kick off CI again

looks good now!

@emilyreff7 emilyreff7 closed this Sep 13, 2019
@emilyreff7 emilyreff7 reopened this Sep 13, 2019
@emilyreff7
Copy link
Contributor Author

these seem like spurious failures, maybe try amending your commit and force pushing to kick off CI again

looks good now!

nvm, accidentally closed so the tests are rerunning now :(

@emilyreff7
Copy link
Contributor Author

these seem like spurious failures, maybe try amending your commit and force pushing to kick off CI again

looks good now!

nvm, accidentally closed so the tests are rerunning now :(

okay, now all good

@toryhaavik toryhaavik merged commit 064b424 into ibis-project:master Sep 13, 2019
costrouc pushed a commit to costrouc/ibis that referenced this pull request Oct 10, 2019
Unlike other operations, not all of the arguments of AsOfJoin are
automatically passed through the validation chain that occurs in
superclass Annotable, since they are not passed into super(). The
result is that whereas other operations with offset-like arguments
(e.g. Lead/Lag) have their offsets' dtypes automatically inferred, the
same does not occur for 'tolerance' in AsOfJoin. We can simply wire
this validation in ourselves.    Additionally, the default unit for
Interval is 's' regardless of the unit of the value of origin. We can
make a better attempt at inferring unit before immediately defaulting
to seconds.
Author: Emily Reff <emily.reff@twosigma.com>

Closes ibis-project#1952 from emilyreff7/hackday and squashes the following commits:

2d7ff71 [Emily Reff] Merge branch 'master' of https://github.com/ibis-project/ibis into hackday
40436e1 [Emily Reff] break up tests
134caf3 [Emily Reff] fix failing tests
91318be [Emily Reff] fix failing tests
c524401 [Emily Reff] fix failing tests
97fa826 [Emily Reff] validate AsOfJoin tolerance and attempt interval unit conversion
@emilyreff7 emilyreff7 deleted the hackday branch March 19, 2021 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants